-
-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add "fileResolve" option for deep import resolving #142
Conversation
@KingSora thanks for the PR, I really appreciate your contribution. I have no time to implement features myself now, so this is a huge help for me. I’ll review the PR in a few days. |
@madyankin looking forward for the review! I've also created another PR which should fix sourcemaps #143 |
@madyankin just a little reminder :) |
@KingSora thanks for pinging me. I have a todo in my Things, but now I'm relocating to another country, so a bit overwhelmed. Will have some time for the review next week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KingSora hey, I found some time to review the PR. I hope you're not demotivated by such a long time waiting for the review. Could you please make a few changes in the PR?
path.resolve(this.root, relativeDir), | ||
newPath | ||
); | ||
(useFileResolve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rewrite this using async/await. I'm thinking of rewriting all the promises into async/await in the future
@@ -50,6 +50,11 @@ declare interface Options { | |||
Loader?: typeof Loader; | |||
|
|||
resolve?: (file: string) => string | Promise<string>; | |||
|
|||
fileResolve?: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for updating the typings
@@ -85,6 +85,15 @@ module.exports = (opts = {}) => { | |||
if (resultPluginIndex === -1) { | |||
throw new Error("Plugin missing from options."); | |||
} | |||
// resolve and fileResolve can't be used together |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for thinking about backwards compatibility. Anyway, I'd prefer to introduce a breaking change and bump the major version. So, let's remove the original resolve
version, replace it with yours, and update the readme. Feel free to use the resolve
option name for this.
@madyankin please leave it open. I'll come back to it as soon as possible (next week possibly) I just need to finish an different project first |
@madyankin on it |
Good day!
I wanted to use the
resolve
option in order to resolve files from thecompose:
import. Unfortunately this option only gives me the possibility to resolve the "first layer" of composes.Consider the following setup:
In this setup the
a.css
file is my entry point. Theresolve
function will only be fired for thecomposes: b from "./b.css"
statement. The "deeper" import insideb.css
is completely opaque.For this usecase I've implemented the
fileResolve
option in a non-breaking manner. This function will be called for allcomposes
statements no matter how deep they are.file
andimporter
, both of which are strings. Thefile
is essentially the part between the brackets after thefrom
keyword (same as the first argument of theresolve
option). Theimporter
is the file from which thiscompose
statements comes from.string | null
or aPromise<string | null>
. The returnedstring
has to be an absolute path. Ifnull
is returned the default resultion takes place.I haven't done that, but with this change I would propose to
deprecate
the currentresolve
option, because the newfileResolve
option is a bit more powerful, still straight forward to use and can still do the same thing as the
resolve
function.The
README
would also need an update, but I would do that after I the feedback from this PR.Fixes: #131